Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/bump mlx #75

Merged
merged 29 commits into from
Dec 11, 2024
Merged

Feature/bump mlx #75

merged 29 commits into from
Dec 11, 2024

Conversation

LeonNissen
Copy link
Contributor

@LeonNissen LeonNissen commented Nov 18, 2024

Update MLX Version

⚙️ Release Notes

Breaking Changes: This pull request introduces additional breaking changes.
Version Updates:

  • mlx-swift has been updated to version 0.18.1.
  • mlx-swift-examples has been updated to version 1.18.1.
  • LLMLocalSchema can now be update with other parameters.
  • The ChatTemplate used by swift-transformers can be overwritten.
  • The LLMContext can be overwritten with a custom context ([[String: String]]).

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Leon Nissen added 17 commits October 15, 2024 18:52
add note to README
fix memory selection
# Conflicts:
#	Package.swift
#	Sources/SpeziLLMLocal/LLMLocalSchema.swift
#	Sources/SpeziLLMLocal/LLMLocalSession+Generate.swift
#	Sources/SpeziLLMLocal/LLMLocalSession+Setup.swift
#	Tests/UITests/TestAppUITests/TestAppLLMLocalUITests.swift
@LeonNissen LeonNissen self-assigned this Nov 18, 2024
@philippzagar
Copy link
Member

philippzagar commented Nov 28, 2024

@LeonNissen #78 tackles a breaking change from the mlx-swift-examples library, feel free to close out the PR as soon as this bump PR is merged! 🚀
Same goes for the issue: #77

@LeonNissen LeonNissen marked this pull request as ready for review December 5, 2024 21:26
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 147 lines in your changes missing coverage. Please review.

Project coverage is 38.52%. Comparing base (8bf0a2f) to head (021fe77).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...urces/SpeziLLMLocal/Mock/LLMLocalMockSession.swift 0.00% 73 Missing ⚠️
...urces/SpeziLLMLocal/LLMLocalSession+Generate.swift 0.00% 37 Missing ⚠️
Sources/SpeziLLMLocal/LLMLocalSession+Update.swift 0.00% 10 Missing ⚠️
...eziLLMLocal/Helpers/LLMContext+FormattedChat.swift 0.00% 8 Missing ⚠️
Sources/SpeziLLMLocal/LLMLocalPlatform.swift 0.00% 7 Missing ⚠️
Sources/SpeziLLMLocal/LLMLocalSchema.swift 0.00% 6 Missing ⚠️
...eziLLMLocal/Configuration/LLMLocalParameters.swift 0.00% 2 Missing ⚠️
Sources/SpeziLLMLocal/LLMLocalSession+Setup.swift 0.00% 2 Missing ⚠️
Sources/SpeziLLM/Models/LLMContextEntity.swift 0.00% 1 Missing ⚠️
Sources/SpeziLLMLocal/LLMLocalSession.swift 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   36.24%   38.52%   +2.28%     
==========================================
  Files          64       65       +1     
  Lines        2503     2355     -148     
==========================================
  Hits          907      907              
+ Misses       1596     1448     -148     
Files with missing lines Coverage Δ
Sources/SpeziLLM/Models/LLMContextEntity.swift 35.00% <0.00%> (ø)
Sources/SpeziLLMLocal/LLMLocalSession.swift 0.00% <0.00%> (ø)
...eziLLMLocal/Configuration/LLMLocalParameters.swift 0.00% <0.00%> (ø)
Sources/SpeziLLMLocal/LLMLocalSession+Setup.swift 0.00% <0.00%> (ø)
Sources/SpeziLLMLocal/LLMLocalSchema.swift 0.00% <0.00%> (ø)
Sources/SpeziLLMLocal/LLMLocalPlatform.swift 0.00% <0.00%> (ø)
...eziLLMLocal/Helpers/LLMContext+FormattedChat.swift 0.00% <0.00%> (ø)
Sources/SpeziLLMLocal/LLMLocalSession+Update.swift 0.00% <0.00%> (ø)
...urces/SpeziLLMLocal/LLMLocalSession+Generate.swift 0.00% <0.00%> (ø)
...urces/SpeziLLMLocal/Mock/LLMLocalMockSession.swift 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bf0a2f...021fe77. Read the comment docs.

Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LeonNissen for the great PR! 🚀 Great to see that SpeziLLM is continuously moving forward!
I had some thoughts in-line, feel free to follow up on them, then we can merge the PR soon (especially in order to fix e.g. #77)
Regarding the failing patch test coverage: No need to worry about that!

Update: As SpeziLLM currently doesn't compile at all (because of the MLX semantic versioning issue described above), I pinned the MLX library versions to specific versions in #80
Please take that into consideration when pinning your MLX dependencies!

Package.swift Outdated Show resolved Hide resolved
Sources/SpeziLLM/Models/LLMContextEntity.swift Outdated Show resolved Hide resolved
Sources/SpeziLLMLocal/LLMLocalSession+Generate.swift Outdated Show resolved Hide resolved
Sources/SpeziLLMLocal/LLMLocalSession+Generate.swift Outdated Show resolved Hide resolved
Sources/SpeziLLMLocal/LLMLocalSession.swift Show resolved Hide resolved
@philippzagar philippzagar added the enhancement New feature or request label Dec 8, 2024
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review @philippzagar! Made some small changes to the tagged versions to clarify semantic versioning expressiveness in the Package file; thank you for all the feedback!

@philippzagar
Copy link
Member

Thank you for the review @philippzagar! Made some small changes to the tagged versions to clarify semantic versioning expressiveness in the Package file; thank you for all the feedback!

Thanks @PSchmiedmayer for looking into that!
Regarding the semantic versioning changes: I pinned both MLX libraries, just to be sure. I didn't use .upToNextMinor() as it is apparently deprecated? See: https://developer.apple.com/documentation/packagedescription/package/dependency/requirement-swift.enum/uptonextminor(from:)
The alternative appears to be: https://developer.apple.com/documentation/packagedescription/package/dependency/package(url:_:)-1r6rc

And yes, the exact: init is definitely a better choice than branch: for sure, thanks!

@PSchmiedmayer
Copy link
Member

@philippzagar The constraint on the Requirement type was deprecated; Version still has a valid static function that we are using in the Package: https://developer.apple.com/documentation/swift/range/uptonextminor(from:). TBH a bit confusing and strange that they don't point to the non-deprecated from the deprecated version 🤷‍♂️

@philippzagar
Copy link
Member

Thank you for the review @philippzagar! Made some small changes to the tagged versions to clarify semantic versioning expressiveness in the Package file; thank you for all the feedback!

Oh I see, was a bit confused as well about that..

@LeonNissen
Copy link
Contributor Author

Thanks @LeonNissen for the great PR! 🚀 Great to see that SpeziLLM is continuously moving forward! I had some thoughts in-line, feel free to follow up on them, then we can merge the PR soon (especially in order to fix e.g. #77) Regarding the failing patch test coverage: No need to worry about that!

Update: As SpeziLLM currently doesn't compile at all (because of the MLX semantic versioning issue described above), I pinned the MLX library versions to specific versions in #80 Please take that into consideration when pinning your MLX dependencies!

Thank you so much for you in-depth feedback! Lets just discuss the final comments and finish up to tag a new version 👍

@philippzagar philippzagar self-requested a review December 11, 2024 09:56
Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for incorporating the feedback @LeonNissen. 🚀
As mentioned, there are definitely some points in there that we should make a bit more flexible and documented in the future, but they are definitely fine for now!

@LeonNissen
Copy link
Contributor Author

Awesome, thank you, @philippzagar! Your feedback has truly improved the PR 🥇

@LeonNissen LeonNissen closed this Dec 11, 2024
@LeonNissen LeonNissen reopened this Dec 11, 2024
@LeonNissen LeonNissen enabled auto-merge (squash) December 11, 2024 16:14
@PSchmiedmayer PSchmiedmayer merged commit fd9f1ca into main Dec 11, 2024
36 of 38 checks passed
@PSchmiedmayer PSchmiedmayer deleted the feature/bump-mlx branch December 11, 2024 17:11
paulhdk pushed a commit that referenced this pull request Dec 16, 2024
# *Update MLX Version*

## ⚙️ Release Notes 
**Breaking Changes**: This pull request introduces additional breaking
changes.
Version Updates:
- mlx-swift has been updated to version 0.18.1.
- mlx-swift-examples has been updated to version 1.18.1.
- `LLMLocalSchema` can now be update with other parameters.
- The `ChatTemplate` used by `swift-transformers` can be overwritten.
- The `LLMContext` can be overwritten with a custom context (`[[String:
String]]`).

## 📝 Code of Conduct & Contributing Guidelines 

By submitting creating this pull request, you agree to follow our [Code
of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md):
- [x] I agree to follow the [Code of
Conduct](https://github.com/StanfordSpezi/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/StanfordSpezi/.github/blob/main/CONTRIBUTING.md).

---------

Co-authored-by: Leon Nissen <>
Co-authored-by: Paul Schmiedmayer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants